-
Notifications
You must be signed in to change notification settings - Fork 4
Remove MultiIndex and Point class #143
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #143 +/- ##
==========================================
+ Coverage 88.21% 88.99% +0.77%
==========================================
Files 89 87 -2
Lines 5246 5071 -175
==========================================
- Hits 4628 4513 -115
+ Misses 618 558 -60 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
| * | ||
| */ | ||
|
|
||
| #define FINE_NODE_EXTRAPOLATED_PROLONGATION() \ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This may be outside the scope of this PR but why are macros used here rather than static inline functions?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes we should probably avoid these macros.
The reason I started using them was, that normal inlined function are slower than actual pasting in the code twice. But I haven't checked that in a while, so we could benchmark if using static inline does provide the same performance.
| namespace ExtrapolatedProlongationTest | ||
| static Vector<double> generate_random_sample_data(const PolarGrid& grid, unsigned int seed) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Does the static remove the need for the namespace?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It is possible to either use the namespace or static to avoid the function collision caused by defining it in multiple .cpp files.
Here I switched to static since it is a bit shorter but the old namespace approach would also work.
| std::uniform_real_distribution<double> dist(-100.0, 100.0); | ||
| for (uint i = 0; i < vector.size(); ++i) { | ||
| vector[i] = dist(gen); | ||
| } | ||
| return vector; | ||
| std::uniform_real_distribution<double> dist(-20.0, 20.0); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why do you test on a narrower set of values now?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This changed because I rewrote the entire google test file (since prolongation0 is no longer available and we need a different test to verify the correctness).
Doesn't really matter what distribution we use.
Merge after: #141
Removes unnecessary MultiIndex and Point class. Cleans up the PolarGrid class and removes the slow unoptimized indexing.
Merge Request - GuideLine Checklist
Guideline to check code before resolve WIP and approval, respectively.
As many checkboxes as possible should be ticked.
Checks by code author:
Always to be checked:
If functions were changed or functionality was added:
If new functionality was added:
If new third party software is used:
If new mathematical methods or epidemiological terms are used:
Checks by code reviewer(s):